refactor: staged writes in private events #19488
Conversation
mverzilli
added
the
ci-no-fail-fast
mverzilli
changed the base branch from
next
to
martin/staged-writes-in-tagging-stores
| public async getPrivateEvents( | ||
| eventSelector: EventSelector, | ||
| filter: PrivateEventStoreFilter, | ||
| jobId: string, |
There was a problem hiding this comment.
Might be a misunderstanding on my part, but do we ever want to provide "transient" data if someone invokes the external API? (I think this method is only ever invoked by consumers, e.g: the wallet)
There was a problem hiding this comment.
The call to getPrivateEvents goes through the queue in part because it triggers the contract private sync.
You're right that this method doesn't need the jobId per se, and in my initial exploration of this I had actually made the argument optional.
But since contract sync is happening and we have the jobId at hand, I ended up deciding for this because it seemed the least convoluted to explain.
There was a problem hiding this comment.
The alternative would be to remove the job id from this function, make it access the db directly and have the call to the store happen after the job queue callback, once the db has been committed. But I agree that this doesn't make a lot of sense. We should maybe add a comment in pxe.ts noting that even though we're returning uncommited data, we commit it immediately after, before returning.
nventuro
left a comment
nventuro
left a comment
There was a problem hiding this comment.
I'm not sure I understand your point re. rollbacks, but other than that this looks good. It's sad that the event store is in poor state but thats' life I guess.
| for (const [eventCommitmentIndex, eventEntry] of this.#getEventLogsInJobStage(jobId)) { | ||
| this.logger.verbose('storing private event log (KV store)', { | ||
| eventCommitmentIndex: eventEntry.eventCommitmentIndex, | ||
| l2BlockNumber: eventEntry.l2BlockNumber, | ||
| l2BlockHash: eventEntry.l2BlockHash, | ||
| txHash: eventEntry.txHash, | ||
| lookupKey: eventEntry.lookupKey, | ||
| }); | ||
|
|
||
| await Promise.all([ |
There was a problem hiding this comment.
If you're already doing promise all here, why not do a flat and then await the entire thing?
So instead of
for event in events
await promise.all([op1(event), op2(event)])
you'd do
await Promise.all(
events.map(event => [op1(event), op2(event)])
.flat()
)
| } | ||
|
|
||
| async #isSeenLog(jobId: string, eventCommitmentIndex: number): Promise<boolean> { | ||
| // getSeenLogsFromJobStage |
There was a problem hiding this comment.
| // getSeenLogsFromJobStage |
| * We don't need staged writes for a rollback since it's handled in the context of a blockchain rewind. | ||
| * An interruption of that process midway through leaves the store in a state in which PXE still will | ||
| * know that it has to rewind, so there's no risk of data integrity corruption in this case. |
Fourth part of the series started with #19445.
This makes the PrivateEventStore work based on staged writes. With this, private events aren't written to persistent storage until PXE decides to commit the job.